Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[monit_syncd][bfn] Use correct syncd launch parameters in monit_syncd #4029

Closed

Conversation

vsenchyshyn
Copy link
Contributor

Signed-off-by: Vitaliy Senchyshyn vsenchyshyn@barefootnetworks.com

- What I did
Fixed launch parameters in monit_syncd for BFN platform so now error log about syncd not running do0esn't wrognly appear when syncd is up.

- How I did it

- How to verify it
Run SONiC on BFN box and verify there is no "'syncd' process is not running" error log when syncd is up and running

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Vitaliy Senchyshyn <vsenchyshyn@barefootnetworks.com>
@vsenchyshyn
Copy link
Contributor Author

@yxieca Please review this.

@yxieca
Copy link
Contributor

yxieca commented Jan 16, 2020

If this is the right argument string on BF platform. By all means go ahead.

I am curious why you put sai.profile under /tmp/?

Can you follow other platform's sample to mount device folder to syncd. If the sai.profile is generated by a template, please follow https://github.com/Azure/sonic-buildimage/blob/master/platform/broadcom/docker-syncd-brcm/start.sh#L45 to put it to a consistent place.

If you need to adjust sai.profile location, please start anther PR. This comment doesn't mean to ask you to change it in this PR.

@yozhao101
Copy link
Contributor

I will submit a PR to fix the name of syncd process in the monit config file.

@jleveque
Copy link
Contributor

I suggest we close this PR in favor of #4033, in which we do not check for any parameters. This solution will be more future-proof, in that if any parameters change, we will not need to make any updates here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants